feat(torii-grpc): add list to member value enum for usage in sdk #2828
Conversation
WalkthroughOhayo, sensei! The changes introduce a new list-based variant to the Changes
Sequence DiagramsequenceDiagram
participant Enum as MemberValue
participant Converter as From Implementation
participant Proto as Proto Types
Enum ->> Converter: Convert List variant
Converter ->> Proto: Create MemberValueList
Converter ->> Proto: Convert each list item
Proto -->> Converter: Return converted list representation
The sequence diagram illustrates how the new list variant is converted from the internal Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🔇 Additional comments (2)crates/torii/grpc/src/types/mod.rs (2)
The recursive nature of
The implementation efficiently converts the list using iterators and handles nested values correctly through recursion. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/torii/grpc/src/types/mod.rs(2 hunks)
🔇 Additional comments (1)
crates/torii/grpc/src/types/mod.rs (1)
154-154: Ohayo! The List variant addition looks good, sensei!
The recursive definition allowing Vec<MemberValue> is well-structured and follows Rust's idiomatic patterns for representing nested data structures.
Let's verify the usage patterns to ensure we handle nested lists appropriately:
✅ Verification successful
Let me analyze the protobuf definition and any existing validation patterns to ensure we have proper handling of nested lists.
Ohayo! The List variant implementation is properly aligned with protobuf definition, sensei!
The implementation correctly maps to the protobuf MemberValueList message type, which is defined with repeated MemberValue values. The conversion logic handles the transformation appropriately, and the protobuf definition supports the nested structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for patterns where List variant is used
rg -A 5 "MemberValue::List"
Length of output: 490
Script:
#!/bin/bash
# Let's check the protobuf definition for MemberValueList
rg -A 10 "message MemberValueList"
# Also check if there are any validation implementations
rg -A 5 "validate.*List"
# Check for any test cases involving List variant
rg -A 10 "test.*List"
Length of output: 684
crates/torii/grpc/src/types/mod.rs
Outdated
| MemberValue::List(list) => member_value::ValueType::List(proto::types::MemberValueList { | ||
| values: list.into_iter().map(|v| proto::types::MemberValue { value_type: Some(v.into()) }).collect(), | ||
| }), |
There was a problem hiding this comment.
💡 Codebase verification
Ohayo! We need to implement the reverse conversion, sensei!
After reviewing the entire file, I can confirm that while we have the conversion from MemberValue to protobuf type, we're missing the reverse conversion (from protobuf to MemberValue). This could lead to issues when deserializing data from the protobuf format.
- Missing implementation for
From<proto::types::MemberValue> for MemberValueorTryFrom<proto::types::MemberValue> for MemberValue - The reverse conversion for the
Listvariant needs to be handled recursively, similar to the forward conversion
🔗 Analysis chain
Implementation looks clean, but let's verify the reverse conversion, sensei!
The conversion to protobuf type is well-implemented using iterators and handles nested values correctly. However, we should ensure the reverse conversion (from proto to MemberValue) is also implemented.
Let's check for the reverse conversion implementation:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the reverse conversion implementation
ast-grep --pattern 'impl $_ for MemberValue {
$$$
fn from($_: proto::types::MemberValue) -> Self {
$$$
}
}'
# Also check for TryFrom implementation
ast-grep --pattern 'impl TryFrom<proto::types::MemberValue> for MemberValue {
$$$
fn try_from($_: proto::types::MemberValue) -> Result<Self, Self::Error> {
$$$
}
}'
Length of output: 296
Script:
#!/bin/bash
# Let's try a broader search for MemberValue implementations
rg -A 10 "impl.*MemberValue"
# Also search for From trait implementations in the file
rg -A 10 "impl.*From.*MemberValue" crates/torii/grpc/src/types/mod.rs
# And search specifically for the List variant conversion
rg -A 5 "MemberValue::List" crates/torii/grpc/src/types/mod.rs
Length of output: 1963
Script:
#!/bin/bash
# Let's search for From implementations in the reverse direction
rg -A 10 "impl From<proto::types::MemberValue.*> for MemberValue"
# Also search for any conversion from proto::types::MemberValueList
rg -A 10 "MemberValueList.*MemberValue"
# And let's check the entire file content to ensure we don't miss any conversion
cat crates/torii/grpc/src/types/mod.rs
Length of output: 16672
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2828 +/- ##
==========================================
- Coverage 55.92% 55.92% -0.01%
==========================================
Files 439 439
Lines 56156 56163 +7
==========================================
- Hits 31408 31407 -1
- Misses 24748 24756 +8 ☔ View full report in Codecov by Sentry. |
glihm
changed the title
Summary by CodeRabbit
New Features
Listvariant to enhance theMemberValuetype, allowing for a collection ofMemberValueitems.Bug Fixes
Listvariant, ensuring proper functionality.